For class variables, lookup type in base classes (#1338, #2022, #2211) by TrueBrain · Pull Request #2380 · python/mypy

TrueBrain · 2016-10-29T16:43:42Z

With this patch, class variables are validated for their type if only the base class defines the type.

Typeshed shows 1 issue in the pyi files. doc is set by MyPy to be 'str'. In stdlib/2/types.pyi, in FunctionType, it is set to func_doc, which is 'Optional[str]'. Because with this patch assignments are checked against their definition, assigning 'Optional[str]' to 'str' is incompatible.

The solution appears simple, by doing types.pyi similar as stdlib/3: not using a level of indirection when assigning these types.

So instead of:

func_doc = ...  # type: Optional[str]
 __doc__ = func_doc

Doing:

__doc__ = ... # type: Optional[str]

If you like, I can make a PR for that change too; but I was first more curious if this PR is correct.

TrueBrain · 2016-10-29T18:18:08Z

I am having a hard time figuring out how TypeVars could be resolved. Any pointers (given the rest of the patch is a good idea) would be very welcome. For now I simply disabled inheriting TypeVars from the base class, and possibly it is even something for another PR.

This avoids looking into the base class of a base class while the type has been changed

gvanrossum · 2016-10-29T20:04:27Z

I'm excited that you're working on this! I'm not sure I can answer your questions (maybe @JukkaL can help). Personally I'm not sure whether type variables declared in a class should ever be overridden. However I don't see any tests for generic classes -- surely there are all sorts of interesting cases when the base class is generic over T and the class variable's type involves T, and then the subclass either substitutes a specific type for T, or a different type variable, etc...

regarding __doc__, I think that should just always be considered Optional[str].

TrueBrain · 2016-10-29T23:22:53Z

Found a way to lookup TypeVars. Added some test cases for it, let me know if you can think of more juicy ones.

Regarding __doc__, it is hard coded in the MyPy codebase as being '__builtins__.str' (mypy/nodes.py). I cannot figure out how to change that to Optional. Hopefully someone can point me in the right direction to fix that issue. Nevermind; turns out __builtin__.pyi also set __doc__ to str again. Will prepare a PR for Typeshed to change all __doc__ to Optional[str]. There are quiet a few str atm.

… subclass

@gvanrossum

python/mypy#2380 showed a discrepancy between object and FunctionType in stdlib2. The first defined __doc__ to be str, the second Optional[str]. As FunctionType depends on object, this is no longer valid. As suggested by @gvanrossum in python/mypy#2380, all __doc__ should be considered Optional. (Final verdict was just to remove most __doc__ attributes since it's inherited from object.)

JukkaL · 2016-10-31T14:23:43Z

You can probably use map_type_from_supertype to translate generic types from a base class to a derived class.

Example where this would be useful:

T = TypeVar('T')

class A(Generic[T]):
    x = None  # type: List[T]
    y = None  # type: List[T]

class B(A[int])
    x = [1]   # ok
    y = ['']  # report error

If that doesn't work for some reason, it's also okay to reject assignments to class variables with type variables in them and file an issue about supporting them.

gvanrossum · 2016-10-31T15:04:09Z

I'm not sure it's a good idea to make those errors. I expect those to be common, as stand-in for instance variables. (Also encouraged by PEP 526.)

…

--Guido (mobile)

TrueBrain · 2016-10-31T18:10:24Z

I indeed found out that map_type_from_supertype does the trick. While adding your example to the test-suite, I noticed I overlooked something (type T worked, but List[T] didn't). Also fixed now with latest commit.

This should implement all cases I could find (including TypeVars). Let me know if you can think up any more test cases to include!

gvanrossum

This is a pretty neat small change! I have a few questions.

gvanrossum · 2016-11-01T23:28:06Z

+class A:
+    a = 1 # type: int
+class B(A):
+    a = None


Hm, this would become invalid with strict none checking. Maybe just use another int value?

gvanrossum · 2016-11-01T23:29:59Z

+    a = "a"
+[out]
+main: note: In class "B":
+main:4: error: Incompatible types in assignment (expression has type "str", variable has type "int")


In a large class hierarchy this error could be pretty mysterious. Can you figure out a way to at least link to the superclass where the variable is declared first? (Even better the exact line, but that may be tricky.)

I wonder if this is an issue for only class variables; in general it would be nice if we can tell where the original declaration came from. As the same issue is already true for when using "self.a = 1" for example. I will fiddle with this a bit, see what I can come up with.

gvanrossum · 2016-11-01T23:31:46Z

+class A:
+    a = None  # type: int
+class B(A):
+    a = None  # type: str


Hm, are you sure that this (without caring about C even) should be allowed? I think it's just as much an error as when no type is given. (However, the declared type here should be allowed to be a subclass of the type declared earlier.)

Class variables are invariant, so the type should be equivalent (is_equivalent) to the base class type. Example:

class A: a = None # type: object def f(self) -> None: self.a = 'x' class B(A): a = None # type: int def g(self) -> None: print(self.a + 1) # ouch, could be str

gvanrossum · 2016-11-02T22:42:32Z

Another idea for more tests: the superclass could have a method; a subclass should not be able to override this with a variable. And vice versa.

…pe of a base class

… other

TrueBrain · 2016-11-05T15:47:10Z

I always love how adding a test case can show you other issues. Fixed all issues I am aware off. I also added that if a superclass defines a @property it can be redefined as variable in subclasses. This mostly because io.pyi uses this. Still investigating if we can show where a type was defined.

TrueBrain · 2016-11-05T17:48:02Z

Splitted off the "find where a type was first declared" into a bug ticket and a PR in my fork. Hope you don't mind, but I would like to resolve it a bit more generic :)

TrueBrain · 2016-11-05T19:15:27Z

Found one additional issue.

https://github.com/python/typeshed/blob/5a2a46d3bd43f5fffb9a6216fe6dedf59ce2f7a1/stdlib/2/ConfigParser.pyi

Line 35 defines args as Tuple[str,str,str]. Line 40 as Tuple[str,str,str,str]. This is of course incompatible, and is now an error. This also happens on line 51 and 58.

I have no idea how to resolve this cleanly, as I cannot figure out what 'args' refers to. Mainly because the Python source does not reference it. Any suggestions would be appreciated.

gvanrossum · 2016-11-05T21:42:49Z

Ah, I see what this is. args is a predefined attribute on BaseException. Its value is always a tuple and its initial value is always the tuple of positional arguments passed to the BaseException constructor. (It can be assigned to as well, but it must always be an iterable, and it is always converted to a tuple.)

I don't know how this ended up in the stubs but the solution is to just remove these from the ConfigParser stub. (For bonus points, change it to Tuple[Any, ...] in BaseException for Python 2 and 3.)

gvanrossum · 2016-11-08T00:14:12Z

Um, you synced typeshed one commit before the fix you applied. You should probably do something like this:

cd typeshed
git co master
git pull
cd ..
git add typeshed
git ci -m 'Sync typeshed'
git push

TrueBrain · 2016-11-08T07:34:31Z

Oops, indeed, I forgot to rebase my master branch against upstream. Silly mistake :D Tnx!

JukkaL · 2016-11-25T11:42:33Z

@TrueBrain This looks good to me. If you fix the merge conflict (should be easy) this can be merged. Optionally, implement/test redefining an attribute with an Any type:

class A:
    a = 1
class B(A):
    a = 'x'  # type: Any  # Should probably be okay

JukkaL · 2016-11-28T12:10:10Z

Added #2500 as a potential follow-up task based on above discussion about showing the class that originally defined the attribute.

#2211)" (#2504) Reverts #2380 The reason is #2503 -- this breaks valid code.

#2510) Continuation of #2380 (which was reverted). Fixes #2503.

@gvanrossum

python/mypy#2380 showed a discrepancy between object and FunctionType in stdlib2. The first defined __doc__ to be str, the second Optional[str]. As FunctionType depends on object, this is no longer valid. As suggested by @gvanrossum in python/mypy#2380, all __doc__ should be considered Optional. (Final verdict was just to remove most __doc__ attributes since it's inherited from object.)

TrueBrain added 2 commits October 29, 2016 18:27

For class variables, lookup type in base classes (#1338, #2022, #2211)

5547c84

Ignore TypeVars from base class when looking up types of class variables

dd2df98

Break even if the base type is a TypeVar

d7373ab

This avoids looking into the base class of a base class while the type has been changed

Support TypeVar when lookup up class variables in a base class

450c12e

TrueBrain mentioned this pull request Oct 30, 2016

Consider __doc__ always Optional. Fixes issues surfaced with python/mypy#2380. python/typeshed#641

Merged

An additional test-case to see if TypeVars are resolved with an extra…

1744b9f

… subclass

Also handle TypeVars in, for example, a List

150361a

gvanrossum reviewed Nov 1, 2016

View reviewed changes

TrueBrain added 6 commits November 5, 2016 12:44

Merge remote-tracking branch 'upstream/master' into class-variable

3871a23

Fix initial test-case

01224ad

Validate if type is defined with an incompatible value against the ty…

8a14697

…pe of a base class

Additional test case to ensure methods/variables don't overwrite each…

08cd704

… other

Also support properties

afab148

Add missing file

8ab3463

This was referenced Nov 5, 2016

GenericBrowser has two parameters swapped around python/typeshed#664

Merged

Show where an "incompatible type" was first declared #2405

Closed

Sync typeshed

e20372e

TrueBrain added 2 commits November 8, 2016 08:31

Sync typeshed

007b303

Merge branch 'master' into class-variable

479430e

Merge branch 'master' into class-variable

ac971f4

TrueBrain added 2 commits November 27, 2016 12:34

Merge branch 'master' into class-variable

8c0138a

Add test case to show class variable type can be overwritten with Any

3518c94

JukkaL merged commit 5ad40b8 into python:master Nov 28, 2016

This was referenced Nov 28, 2016

Assignment to next in class body of Iterator elicits error (in PY2 mode) #2503

Closed

Revert "For class variables, lookup type in base classes (#1338, #2022, #2211)" #2504

Merged

gvanrossum added a commit that referenced this pull request Nov 28, 2016

Revert "For class variables, lookup type in base classes (#1338, #2022,

506113c

#2211)" (#2504) Reverts #2380 The reason is #2503 -- this breaks valid code.

TrueBrain mentioned this pull request Nov 30, 2016

For class variables, lookup type in base classes (#1338, #2022, #2211) #2510

Merged

gvanrossum pushed a commit that referenced this pull request Jan 27, 2017

For class variables, lookup type in base classes (#1338, #2022, #2211) (

b759293

#2510) Continuation of #2380 (which was reverted). Fixes #2503.

Uh oh!

Uh oh!

Conversation

TrueBrain commented Oct 29, 2016

Uh oh!

TrueBrain commented Oct 29, 2016

Uh oh!

gvanrossum commented Oct 29, 2016

Uh oh!

TrueBrain commented Oct 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JukkaL commented Oct 31, 2016

Uh oh!

gvanrossum commented Oct 31, 2016 via email

Uh oh!

TrueBrain commented Oct 31, 2016

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

gvanrossum Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

gvanrossum Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

TrueBrain Nov 5, 2016

Choose a reason for hiding this comment

Uh oh!

gvanrossum Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

JukkaL Nov 25, 2016

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented Nov 2, 2016

Uh oh!

TrueBrain commented Nov 5, 2016

Uh oh!

TrueBrain commented Nov 5, 2016

Uh oh!

TrueBrain commented Nov 5, 2016

Uh oh!

gvanrossum commented Nov 5, 2016

Uh oh!

gvanrossum commented Nov 8, 2016

Uh oh!

TrueBrain commented Nov 8, 2016

Uh oh!

JukkaL commented Nov 25, 2016

Uh oh!

JukkaL commented Nov 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TrueBrain commented Oct 29, 2016 •

edited

Loading